-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cryptoconditions (RIPD-1139) #2170
Conversation
Would it be possible to generate this code at build time instead of committing it, similar to protobuf code? |
@MarkusTeufelberger Yes, it would be possible. My biggest concern is the script requires several python libraries and python 3 (python 2 support may be easy). It's also useful (tho not required) to run clang-format over the generated code or it will be very difficult to see what's going on. Making sure these deps are installed on all systems that want to build rippled is a headache. It also complicates the build scripts. Tho that's less of a concern. |
If it is not intended for humans, why format it? You'll anyways likely need a build step that verifies that your version of the generated code is still identical to what would be generated in case the generation script changes, so you'll anyways somehow need to encode the code generation in a build script. Which commit contains the actual script? Skipping through all of them seem to be relatively large, so I suspect these are the ones containing generated code... |
@MarkusTeufelberger The |
Thanks, I'll take a look. It would be great if you could also check which exact library versions you installed using conda. |
@MarkusTeufelberger I'm using the following versions: |
src/ripple/conditions/impl/Der.cpp
Outdated
} | ||
|
||
size_t | ||
contentLengthLength(std::uint64_t v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get an error here from clang (mac):
src/ripple/conditions/impl/Der.cpp|175 col 1 error| functions that differ only in their return type cannot be overloaded
|| contentLengthLength(std::uint64_t v)
|| ^
src/ripple/conditions/impl/Der.h|521 col 1| note: previous declaration is here
|| contentLengthLength(std::uint64_t);
|| ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mellery451 Thanks. I just pushed a patch that should fix this (changed decl return type from size_t
to uint64_t
.
Codecov Report
@@ Coverage Diff @@
## develop #2170 +/- ##
===========================================
+ Coverage 70.05% 70.51% +0.46%
===========================================
Files 689 699 +10
Lines 50724 52046 +1322
===========================================
+ Hits 35533 36702 +1169
- Misses 15191 15344 +153
Continue to review full report at Codecov.
|
raise ValueError('Can only set bits between 0 and 31') | ||
result[b] = 1 | ||
# reverse for big endian | ||
return tuple(result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm - I feel like one of these return statements should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
def set_msg_self_and_children(self, msg): | ||
''' | ||
Set the message of a fulfillment so it suceeds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: suceeds
--> succeeds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
''' | ||
Set the message of a fulfillment so it suceeds. | ||
This will set the message of child fulfillments so they | ||
will suceeds as well, in particular, this means prefix subcondition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: suceeds
--> succeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -0,0 +1,1217 @@ | |||
#!/usr/bin/env python | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we have a one-line description of what this script is for and if/when it should be executed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -333,6 +333,7 @@ class PreimageSha256_test : public beast::unit_test::suite | |||
log << "Fulfillment deserialize error: " << std::get<0>(x) << " " << ec.message() << '\n'; | |||
} | |||
auto const c = Condition::deserialize (hexblob(std::get<2>(x)), ec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused about c
here - do we care about the value? Should we verify correctness for it , or just ignore it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition here does not match the fulfillment. They were just dummy values used to test the parser (if you look at the fingerprint in the condition, it's 4849505152535455484950515253545548
, which is clearly not a valid fingerprint.
The history is this test predated the generated tests (which do check that the conditions match the fulfillments), and I decided to keep the old tests. Having said all that, I do agree this should be improved. Now that I have python scripts, I'll fix the conditions so they match the fulfillments as expected and change the test. A second option would be to just remove this file (as the generated tests are better).
condition_logger = logging.getLogger('condition') | ||
|
||
condition_test_template_prefix = \ | ||
''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a comment at the top indicating that the file was generated and "DO NO EDIT"? Or are we okay with people editing the generated sources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/ripple/conditions/impl/Der.h
Outdated
length and sort order so they do not need to be recomputed. Note that storing | ||
values in the cache is type dependent, and the address of the variable must be | ||
stable while encoding. It makes sense to cache higher level values, but not | ||
primitives. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doc is very clear and helpful 👍
src/test/conditions/Der_test.cpp
Outdated
{ | ||
test(i->first, i->second); | ||
using Traits = cryptoconditions::der::DerCoderTraits<std::string>; | ||
this->BEAST_EXPECT(Traits::compare(i->first, i->first, dummy) == 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any particular reason for this->BEAST_EXPECT(...
vs. just BEAST_EXPECT(...
? It doesn't bother me either way, just wondering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The this
is needed in a lambda. I've forgotten the history of this code, but it's likely it used to live in a lambda and when I moved it outside the lambda I forgot to remove the this
.
Will fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Pushed a commit to support clang's fuzzing sanitizer (requires clang 5) |
Great idea using libFuzzer for this. 👍 |
CMakeLists.txt
Outdated
|
||
add_executable(fulfillment_fuzzer ${FuzzerSrc} | ||
${CMAKE_SOURCE_DIR}/src/fuzzers/Conditions_fuzz_test.cpp) | ||
target_link_libraries(fulfillment_fuzzer ${OPENSSL_LIBRARIES} ${SANITIZER_LIBRARIES}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks to me like SANITIZER_LIBRARIES
is no longer being set, so should we just get rid of it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed (removed SANITIZER_LIBRARIES). Although I might bring that back for gcc builds if there are complaints (clang doesn't need it).
CMakeLists.txt
Outdated
-I"${CMAKE_SOURCE_DIR}/"src/ed25519-donna) | ||
|
||
add_executable(fulfillment_fuzzer ${FuzzerSrc} | ||
${CMAKE_SOURCE_DIR}/src/fuzzers/Conditions_fuzz_test.cpp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could Conditions_fuzz_test.cpp
just be added to FuzzerSrc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed (and an explanation - I originally had two separate "main" functions, one for testing fulfullments and one for conditions. When I merged them into a single file I should have also fixed the build script).
CMakeLists.txt
Outdated
@@ -447,16 +447,18 @@ if (WIN32 OR is_xcode) | |||
group_sources(Builds) | |||
endif() | |||
|
|||
if(unity) | |||
if (NOT fuzzer_conditions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion, but why not just allow rippled
to be a target? You can still just build the fuzzed exes by name if you don't want to build rippled
e.g. make fulfillment_fuzzer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
CMakeLists.txt
Outdated
|
||
add_executable(condition_fuzzer ${FuzzerSrc} | ||
${CMAKE_SOURCE_DIR}/src/fuzzers/Conditions_fuzz_test.cpp) | ||
target_link_libraries(condition_fuzzer ${OPENSSL_LIBRARIES} ${SANITIZER_LIBRARIES}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not used libFuzzer myself, but docs indicate that you still need to link with libFuzzer.a
- so we might need to add that here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's building fine for me without it. Clang links in the correct lib if you link with -fsanitize=fuzzer,address
. (The docs for AddressSanitizer say this explicitly: https://clang.llvm.org/docs/AddressSanitizer.html). You're right, I can't find the same snippet with the FuzzerSanitizer, but it is acting the same way as AddressSanitizer.
My motivation for removing the explicit link is I was getting errors about linking in incompatible sanitizer libraries. The easiest way for me to resolve this was to let clang choose the right library for me.
@@ -518,6 +520,40 @@ foreach(target IN LISTS targets) | |||
link_common_libraries(${target}) | |||
endforeach() | |||
|
|||
if (fuzzer_conditions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is much need for a user-defined variable for this since you can just set it based on the san
values, something like: mellery451@a303c0f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep this variable. If we add other fuzzers I think we want to control which fuzzers we are building. I don't mind the slightly clunkier build step for this because it's an uncommon build target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive stuff with the serialization design and testing. I wish there were a standard set of test data for DER encoders that we could also run through the ser/deser classes, but I could not find anything. I think the fuzzing is great as well.
As for the code generation thing, here's a commit that does this: mellery451@b19911d . I've tested on mac/linux and it does the basic job. I would not be surprised if there are fixes required for windows (esp. the python commands). It's cmake only, so we should only consider it once we finally drop scons.
rebased onto 0.80.0-b3 |
Rebased onto 0.80.0-b4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I FINALLY FINISHED!
src/test/basics/Slice_test.cpp
Outdated
BEAST_EXPECT(*s.data() == 42); | ||
s = ms; // can assign an immutable slice from a mutable slice | ||
auto const svs = makeSlice(mutValues); // can create a slice from a boost small vector | ||
BEAST_EXPECT(*svs.data() == 42); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a call to the new push_back
?
ms.push_back(37);
BEAST_EXPECT(mutValues[0] == 37);
BEAST_EXPECT(*ms.data() == 1);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a028ad7 [fold] Responding to Ed PR feedback 1
src/ripple/conditions/impl/Der.h
Outdated
void | ||
encodeContentLength(MutableSlice& dst, std::uint64_t v, std::error_code& ec); | ||
|
||
/** return the number of bytes required to encode a the given content length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Extra article a the
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a028ad7 [fold] Responding to Ed PR feedback 1
src/ripple/conditions/impl/Der.h
Outdated
are added to the stream using the `<<` operator. After all the values are | ||
added to the encoder, it must be terminated with a call to `eos()`. As a | ||
convenience, there is a special variable called `eos` that when streamed will | ||
call the streams `eos()` function. Typically, the code to encode values to a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: stream's
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a028ad7 [fold] Responding to Ed PR feedback 1
src/ripple/conditions/impl/Der.h
Outdated
The decode class has an interface similar to a c++ output stream. Values are | ||
decoded from the stream using the `>>` operator. After all the values are | ||
decoded, it must be terminated with a call to `eos()`. As a convenience, there | ||
is a special variable called `eos` that when streamed will call the streams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit again: stream's
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a028ad7 [fold] Responding to Ed PR feedback 1
src/ripple/conditions/impl/Der.h
Outdated
if (!isSigned && slice.size() == sizeof(T) + 1 && slice[0]) | ||
{ | ||
// since integers are coded as two's complement, the first byte may | ||
// be zero for unsigned reps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must be zero
???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it may be zero, but it is not necessarily zero. For example, the unsigned value one is encoded as 0x1. If the leading 8th bit of a positive value is 1, we need to add a leading 0 to distinguish it from a negative value.
src/ripple/conditions/impl/Der.h
Outdated
auto const& e = std::get<index>(elements); | ||
using ElementTraits = DerCoderTraits<std::decay_t<decltype(e)>>; | ||
// visual studio can't handle childNum = index, use decltype |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you updated the type of index
without updating any other code are these last 2 comments still true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a028ad7 [fold] Responding to Ed PR feedback 1
@@ -511,20 +511,28 @@ endmacro() | |||
|
|||
macro(setup_build_boilerplate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit message is marked fold, but please don't lose the instructions for the fuzzer when this is squashed.
@@ -511,20 +511,28 @@ endmacro() | |||
|
|||
macro(setup_build_boilerplate) | |||
if (NOT WIN32 AND san) | |||
add_compile_options(-fsanitize=${san} -fno-omit-frame-pointer) | |||
STRING(REPLACE ";" "," comma_san "${san}") | |||
add_compile_options(-fsanitize=${comma_san} -fno-omit-frame-pointer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we can't spec to pass a comma on the command line instead of the semicolon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. There are places where we want to treat ${san}
like a list and iterate through its individual values - so we need a semi-colon. There are other places where we need to separate them with a comma because that's how the compiler wants them. So if we passed them with a comma on the command line we'd have to replace that comma with a semi-colon in other places.
src/fuzzers/Conditions_fuzz_test.cpp
Outdated
static int fileNum = 0; | ||
std::ofstream ofs; | ||
char fileName[32]; | ||
sprintf(fileName, "logic_error%d.dat", fileNum++); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the intention here is to prevent overwriting an existing file, I would suggest a loop to check that the file exists, and incrementing again if it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a028ad7 [fold] Responding to Ed PR feedback 1
@@ -67,6 +84,9 @@ | |||
ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF | |||
OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. | |||
*/ | |||
|
|||
// THIS FILE WAS AUTOMATICALLY GENERATED -- DO NOT EDIT | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is not in the generated test files. I assume that means you need to regenerate them. (See also my earlier comment about scripting the generation.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the docs build problem, these changes look great. I look forward to the next round of changes.
@@ -124,6 +124,7 @@ INPUT = \ | |||
../src/ripple/app/tx/applySteps.h \ | |||
../src/ripple/app/tx/impl/InvariantCheck.h \ | |||
../src/ripple/app/consensus/RCLValidations.h \ | |||
../src/ripple/conditions/impl/Der.h \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted elsewhere, the docs fail to build now.
src/ripple/conditions/Fulfillment.h
Outdated
/** encode the contents used to calculate a fingerprint | ||
|
||
@note Most cryptoconditions (excepting preimage) calculate their | ||
fingerprints by encoding into a ans.1 der format and hashing the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"ASN.1" and "DER"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in ad0de5e [fold] Address feedback from review:
src/ripple/conditions/Condition.h
Outdated
Type t, | ||
std::uint32_t c, | ||
std::array<std::uint8_t, 32> const& fp, | ||
std::bitset<5> const& s = std::bitset<5>{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not simply std::bitset<5> s = {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in ad0de5e [fold] Address feedback from review:
* Add a class for a mutable linear buffer * Add support to make a slice from a `boost::container::small_vector`
* Note: requires clang's fuzzer sanitizer (included in clang 5) * Python script to create a fuzz corpus * Allow python script to run outside ipython * Sample cmake: cmake -Dfuzzer_conditions=ON -Dsan='fuzzer;address' -Dtarget=clang.debug .. * Sample Run: ./fulfillment_fuzzer <path_to_corpus_dir> -max_len=5000 -jobs=3 * There are separate executables for fulfillment and conditions.
I pushed another set of changes. I still have two issues left to address: the failing doc generation and dealing with the automatically generated cpp test files. |
* Split Der.{h,cpp} into multiple files and reord includes * rename asn.1 -> ASN.1; der -> DER * std::bitset<5> s = {} * Add comment justifying Ed25519 cost magic number
There is some good news about the doc generation. Since you split Der.h into three files, and doxygen doesn't follow includes, the docs build now. The bad news is that they don't pick up any of the DER code. :) But there's more good news! Because I could add those three files to |
I tracked down the documentation errors. There are three places that confuse document generation. The problem cases are:
I tried some workarounds with My solution is to not generate docs for all three cases (I wrapped the entire definition in |
Jenkins Build SummaryBuilt from this commit Built at 20190615 - 08:57:51 Test Results
|
This code needs to be rebased and resubmitted for consideration in the future. |
This PR is not as large as it first looks. There is a large amount of
automatically generated code that is used for tests. Reviewing that code is not
necessary (reviewing the script that generates the tests, and the code output for
a single test should be enough).
Helpful references:
Cryptoconditions spec: https://tools.ietf.org/html/draft-thomas-crypto-conditions-02
ASN.1 spec: https://www.itu.int/ITU-T/studygroups/com17/languages/X.690-0207.pdf
Informal doc on ASN.1: http://luca.ntop.org/Teaching/Appunti/asn1.html
Web site to encode/decode ASN.1: http://asn1-playground.oss.com/
Note:
The max cryptoconditions size is larger than I'd like. This is because Rsa CCs
are relatively large, and without a large limit I can't write tests for
thresholds CCs that contain RsaSha256 CCs.